Skip to content

fix(front): test suite duration/sorting #645

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(front): test suite duration/sorting #645

wants to merge 1 commit into from

Conversation

@marco2216
Copy link
Contributor

@marco2216 marco2216 commented Oct 10, 2025

📝 Description

Test suite durations are not necessarily the sum of the tests that make up the suite. If using Jest, in most cases the test suite time is longer than the sum of the individual tests.
There's two changes here because

  1. I would argue that the test suite duration should not be changed even when hiding some tests.
  2. suite.syncSummary was being called which mutates the suite object, so the suite duration was being updated after the sorting happened, which means that the sorting was inconsistent with the rendered durations in the UI.

You can see the issue here - sorting is wrong. I found the issue by comparing with the junit.xml test results file.
image

Also, as a general comment, it's a very bad idea to mutate things when writing React code, it makes it extremely hard to understand what happens with state. https://react.dev/learn/updating-objects-in-state#treat-state-as-read-only :)

✅ Checklist

  • I have tested this change (can't figure out how to run this locally, but I'm pretty sure this is a fix - would be great if you could test it)
  • This change requires documentation update

@github-project-automation github-project-automation bot moved this to Backlog in Roadmap Oct 10, 2025
@marco2216 marco2216 changed the title fix test suite duration/sorting fix(front): test suite duration/sorting Oct 10, 2025
@marco2216 marco2216 force-pushed the fix/test-suite-duration branch from 65a455a to d45a79f Compare October 10, 2025 14:16
@marco2216 marco2216 marked this pull request as ready for review October 10, 2025 14:17
@hamir-suspect
Copy link
Contributor

hamir-suspect commented Oct 30, 2025

You make a good point with this change 👍
To make it more flexible, you could make this behavior a user preference so people who depend on aggregated durations can opt back into the recompute flow.

Here’s one way to approach it:

  • Add a respectRunnerDuration (or similar) boolean to the filter state in front/assets/js/test_results/stores/filter.ts:31, persist it in localStorage, and wire up reducer actions to flip it.
  • Expose that flag as a new checkbox under “Display preferences” in front/assets/js/test_results/components/filters.tsx:140, dispatching the toggle when the user clicks.
  • Teach Suite.syncSummary to accept the flag (e.g. syncSummary({ keepRunnerDuration: true })) and either reuse this.summary.duration or recompute from this.tests.reduce(...) in front/assets/js/test_results/types/suite.ts:58.
  • Wherever syncSummary() runs (front/assets/js/test_results/components/test_explorer.tsx:134 and front/assets/js/test_results/types/report.ts:48), pass the current filter-state flag so both suite and report summaries stay consistent.

@marco2216
Copy link
Contributor Author

marco2216 commented Oct 30, 2025

@hamir-suspect What is the use case we're trying to cater to here? I can't come up with a reason that it's useful to see the "calculated" duration.

@marco2216
Copy link
Contributor Author

marco2216 commented Nov 19, 2025

@hamir-suspect did you see my message?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hamir-suspect hamir-suspect Awaiting requested review from hamir-suspect hamir-suspect is a code owner

@skipi skipi Awaiting requested review from skipi skipi is a code owner

@VeljkoMaksimovic VeljkoMaksimovic Awaiting requested review from VeljkoMaksimovic

@forestileao forestileao Awaiting requested review from forestileao forestileao is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

Roadmap
Status: Backlog

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@marco2216 @hamir-suspect